Skip to content

Conversation

@iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Dec 1, 2025

The aim of this PR is twofold:

  • Reduce the number of highly similar TMP|VAR handlers
  • Avoid ZVAL_DEREF in most of these cases

This is achieved by guaranteeing that all zend_compile_expr() calls, as well as all other compile calls with BP_VAR_R, will result in a TMP variable. This implies that the result will not contain an IS_INDIRECT or IS_REFERENCE value, which was mostly already the case, with two exceptions:

  • Calls to return-by-reference functions. Because return-by-reference functions are quite rare, this is solved by delegating the DEREF to the RETURN_BY_REF handler, which will examine the stack to check whether the caller expects a VAR or TMP to understand whether the DEREF is needed.

  • By-reference assignments, including both $a = &$b, as well as [&$a] = $b. When the result of these expressions is used in a BP_VAR_R context, it will be passed to a new ZEND_DEREF opcode beforehand. This is exceptionally rare.

Preliminary testing shows a 1.1% wall time improvement in Symfony Demo and roughly 0.5% in Wordpress. Edit: Sadly I can now only measure a 0.15% improvement for Symfony, but 0.8% for Wordpress. Zend/bench.php improves by ~3% in my tests. There seems to be quite a bit of volatility involved, potentially in relation to binary layout. Regardless, I think this is unlikely to cause true slowdowns for code that doesn't use return-by-ref.

TODOs:

  • Verify this doesn't break important optimizations
  • Use zend_unwrap_reference(). I missed this function, I was looking for a macro.
  • Check why JIT i-count regresses. Edit: Symfony Demo now reduces jitted code by 0.35%, Wordpress by 0.02%.
  • Try to replace DEREF with QM_ASSIGN.
  • Add checks to R/IS compile-paths, asserting no VARs are generated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

if ((t1 & (MAY_BE_ANY|MAY_BE_UNDEF)) == MAY_BE_ARRAY && MAY_BE_EMPTY_ONLY(t1)) {
return false;
}
return true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YIELD_FROM from generators can throw if the generator is closed, even for empty arrays. So this optimization may be unsound.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not mind doing the inverse - changing YIELD_FROM to not throw on empty array, though...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking into account the "closed" generator, this is wrong.

@iluuu1994 iluuu1994 marked this pull request as ready for review December 4, 2025 21:14
@iluuu1994 iluuu1994 requested a review from dstogov as a code owner December 4, 2025 21:14
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should work, but the effect is not great.
(I see 30KB reduction in PHP code size and very slight performance difference).

The path breaks Symfony demo with function JIT (probably because of missing changes in FETCH_(DIM|OBJ)_FUNC_ARG handlers). This needs to be fixed of course.

MAKE_NOP(opline);
++(*opt_count);
if (src->op1_type & (IS_VAR|IS_TMP_VAR)) {
src->opcode = ZEND_FREE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing QM_ASSIGN with FREE opens possibility to deeper application of the same optimization.

if ((t1 & (MAY_BE_ANY|MAY_BE_UNDEF)) == MAY_BE_ARRAY && MAY_BE_EMPTY_ONLY(t1)) {
return false;
}
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking into account the "closed" generator, this is wrong.

@dstogov
Copy link
Member

dstogov commented Dec 22, 2025

I think the patch makes sense, but still has problems.
The executor changes looks fine (see minor comments).
I'm not able to analyse zend_compile.c changes.
I see introduced memory leaks related to NEW on Symfony community tests.

@iluuu1994
Copy link
Member Author

I see introduced memory leaks related to NEW on Symfony community tests.

Thank you, I missed those. I'll have a look.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions
Copy link

AWS x86_64 (c7i.24xl)

Attribute Value
Environment aws
Instance type c7i.metal-24xl (dedicated)
Architecture x86_64
CPU Intel(R) Xeon(R) Platinum 8488C, 48 cores @ 2400 MHz
CPU settings disabled deeper C-states, disabled turbo boost, disabled hyper-threading
RAM 188 GB
Kernel 6.1.158-178.288.amzn2023.x86_64
OS Amazon Linux 2023.9.20251117
GCC 14.2.1
Time 2026-01-28 17:38:56 UTC
Job details https://github.com/php/php-src/actions/runs/21448985323 (Artifacts)
Changeset https://github.com/php/php-src/compare/62c94b6926..39de75b32a

Laravel 12.11.0 demo app - 100 consecutive runs, 50 warmups, 100 requests (sec)

PHP Min Max Std dev Rel std dev % Mean Mean diff % Median Median diff % Skewness Z-stat P-value Memory
PHP - baseline@62c94b6 0.44725 0.45399 0.00090 0.20% 0.44788 0.00% 0.44771 0.00% 6.022 0.000 1.000 27.00 MB
PHP - vm-early-deref 0.44585 0.44924 0.00040 0.09% 0.44650 -0.31% 0.44646 -0.28% 3.506 11.902 0.000 27.01 MB

Symfony 2.8.0 demo app - 100 consecutive runs, 50 warmups, 100 requests (sec)

PHP Min Max Std dev Rel std dev % Mean Mean diff % Median Median diff % Skewness Z-stat P-value Memory
PHP - baseline@62c94b6 0.76487 0.77847 0.00313 0.40% 0.77414 0.00% 0.77533 0.00% -1.695 0.000 1.000 27.01 MB
PHP - vm-early-deref 0.76329 0.77703 0.00299 0.39% 0.77259 -0.20% 0.77369 -0.21% -1.762 6.739 0.000 27.01 MB

Wordpress 6.9 main page - 100 consecutive runs, 20 warmups, 20 requests (sec)

PHP Min Max Std dev Rel std dev % Mean Mean diff % Median Median diff % Skewness Z-stat P-value Memory
PHP - baseline@62c94b6 0.66303 0.66616 0.00043 0.06% 0.66415 0.00% 0.66411 0.00% 0.877 0.000 1.000 27.01 MB
PHP - vm-early-deref 0.65959 0.68008 0.00201 0.30% 0.66069 -0.52% 0.66048 -0.55% 9.251 11.971 0.000 27.01 MB

bench.php - 100 consecutive runs, 10 warmups, 2 requests (sec)

PHP Min Max Std dev Rel std dev % Mean Mean diff % Median Median diff % Skewness Z-stat P-value Memory
PHP - baseline@62c94b6 0.42315 0.44177 0.00205 0.48% 0.42656 0.00% 0.42640 0.00% 4.242 0.000 1.000 27.01 MB
PHP - vm-early-deref 0.42300 0.44239 0.00230 0.54% 0.42637 -0.04% 0.42618 -0.05% 3.405 0.705 0.481 27.01 MB

The aim of this PR is twofold:

- Reduce the number of highly similar TMP|VAR handlers
- Avoid ZVAL_DEREF in most of these cases

This is achieved by guaranteeing that all zend_compile_expr() calls, as well as
all other compile calls with BP_VAR_{R,IS}, will result in a TMP variable. This
implies that the result will not contain an IS_INDIRECT or IS_REFERENCE value,
which was mostly already the case, with two exceptions:

- Calls to return-by-reference functions. Because return-by-reference functions
  are quite rare, this is solved by delegating the DEREF to the RETURN_BY_REF
  handler, which will examine the stack to check whether the caller expects a
  VAR or TMP to understand whether the DEREF is needed. Internal functions will
  also need to adjust by calling the zend_return_unwrap_ref() function.

- By-reference assignments, including both $a = &$b, as well as $a = [&$b]. When
  the result of these expressions is used in a BP_VAR_R context, the reference
  is unwrapped via a ZEND_QM_ASSIGN opcode beforehand. This is exceptionally
  rare.

Closes phpGH-20628
@iluuu1994 iluuu1994 merged commit 6173a9a into php:master Jan 31, 2026
10 checks passed
@mbeccati
Copy link
Contributor

mbeccati commented Feb 1, 2026

@iluuu1994 I'm afraid there are issues with this commit. In my CI both Symfony and Revive Adserver cannot run the test suite.

Symfony 7.4 doesn't even start:

# ../php-src/sapi/cli/php ./phpunit
php: /home/matteo/OSS/php-src/Zend/zend_compile.c:12306: zend_compile_var_inner: Assertion `ast->child[0]->kind == ZEND_AST_ARRAY && zend_propagate_list_refs(ast->child[0])' failed.
[1]    69836 IOT instruction (core dumped)  ../php-src/sapi/cli/php ./phpunit

In order to replicate just clone symfony 7.4, run composer install, then run ./phpunit with a freshly compiled php from master.

In the Revive Adserver suite, I'm getting:

Fatal error: Uncaught Error: SimpleErrorTrappingInvoker::__construct(): Argument #1 ($invoker) could not be passed by reference in /home/runner/work/php-latest-builds/php-latest-builds/lib/simpletest/test_case.php:109

In this case it might just be some deprecation being promoted to error perhaps. Code is very much legacy, so it might just be something that needs to be adjusted on our side. I will report back my findings on this.

@iluuu1994
Copy link
Member Author

@mbeccati Thanks for letting me know. I'll have a look today. I did test this change with nightly, which runs all Symfony tests (along with many others).

@mbeccati
Copy link
Contributor

mbeccati commented Feb 1, 2026

@iluuu1994 Thanks! I can confirm that the Revive Test Suite fails to run because of:

PHP Notice:  Only variables should be passed by reference in /home/matteo/OSS/php-src/r.php on line 8

vs:

Fatal error: Uncaught Error: A::__construct(): Argument #1 ($o) could not be passed by reference in /home/matteo/OSS/php-src/r.php:8

which I'm totally OK with, but will probably need to be listed in UPGRADING (or RFC?).

Simple enough to replicate:

<?php

class A {
   public function __construct(&$o) {
   }
}

var_dump(new A(new \StdClass()));

@iluuu1994
Copy link
Member Author

@mbeccati Changing behavior was not the intention. I'll make sure this keeps working. The first case might just be a bad assertion, we'll see.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Feb 1, 2026

@mbeccati The first issue should be solved. It was just a faulty assertion ("surely adding this code after testing is fine, right?"). The other will take more consideration. Sadly, PHP has inconsistent error types when passing non-vars as references: https://3v4l.org/lE72G Maybe it wouldn't be such a bad idea to unify this case after all, but that would need an RFC. I'll see how easy it is to find a resolution tonight, I'm out of time for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants